Handle mapping to Option in `map_flatten` lint #5846

dima74 · 2020-07-25T17:31:48Z

The existing map_flatten lint suggests changing expr.map(...).flatten() to expr.flat_map(...) when expr is Iterator. This PR changes suggestion to filter_map instead of flat_map when mapping to Option, because it is more natural

Also here are some questions:

If expression has type which implements Iterator trait (match_trait_method(cx, expr, &paths::ITERATOR) == true), how can I get type of iterator elements? Currently I use return type of closure inside map, but probably it is not good way
I would like to change suggestion range to cover only .map(...).flatten(), that is from:

    let _: Vec<_> = vec![5_i8; 6].into_iter().map(|x| 0..x).flatten().collect();
                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try using `flat_map` instead: `vec![5_i8; 6].into_iter().flat_map

to

    let _: Vec<_> = vec![5_i8; 6].into_iter().map(|x| 0..x).flatten().collect();
                                             ^^^^^^^^^^^^^^^^^^^^^^^^ help: try using `flat_map` instead: `.flat_map(|x| 0..x)`

Is it ok?

Is map_flatten lint intentionally in pedantic category, or could it be moved to complexity?

changelog: Handle mapping to Option in map_flatten lint

rust-highfive · 2020-07-25T17:31:50Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @yaahc (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

flip1995 · 2020-07-26T16:04:33Z

r? @flip1995

flip1995

Is map_flatten lint intentionally in pedantic category, or could it be moved to complexity?

Yes this is intentional.

I would like to change suggestion range to cover only .map(...).flatten()

You can get the spans of all methods involved like here:

rust-clippy/clippy_lints/src/methods/mod.rs

Line 1415 in 2e0f8b6

["fold", ..] => lint_unnecessary_fold(cx, expr, arg_lists[0], method_spans[0]),

rust-clippy/clippy_lints/src/methods/mod.rs

Lines 2200 to 2203 in 2e0f8b6

    
           span_lint_and_sugg( 
        
               cx, 
        
               UNNECESSARY_FOLD, 
        
               fold_span.with_hi(expr.span.hi()),

Currently I use return type of closure inside map, but probably it is not good way

I think this is indeed the best way, let's add some more tests for it.

clippy_lints/src/methods/mod.rs

tests/ui/map_flatten.rs

clippy_lints/src/methods/mod.rs

tests/ui/map_flatten.rs

dima74 · 2020-07-30T20:40:00Z

@flip1995 Thank you for review! I update code. I am not sure why CI is not passed, couldn't find anything related to my changes in logs

flip1995

I have no idea why CI failed. it also passes locally on my end. 🤔

flip1995 · 2020-07-30T21:06:37Z

clippy_lints/src/methods/mod.rs

            cx,
            MAP_FLATTEN,
-            expr.span,
+            expr.span.trim_start(map_args[0].span).unwrap(),


I would prefer, if you use map_args[0].span.to_hi(expr.span.hi()). That way, no unwrap is necessary.

flip1995 · 2020-07-30T21:08:34Z

clippy_lints/src/methods/mod.rs

+            // `(...).map(...)` has type `impl Iterator<Item=impl Iterator<...>>
+            "flat_map"
+        };
+        let msg = "called `map(..).flatten()` on an `Iterator`";


Binding this isn't necessary anymore.

flip1995 · 2020-08-04T09:11:16Z

@bors r+ rollup=always

bors · 2020-08-04T09:11:17Z

📌 Commit d4ba561 has been approved by flip1995

…flip1995 Handle mapping to Option in `map_flatten` lint Fixes rust-lang#4496 The existing [`map_flatten`](https://rust-lang.github.io/rust-clippy/master/index.html#map_flatten) lint suggests changing `expr.map(...).flatten()` to `expr.flat_map(...)` when `expr` is `Iterator`. This PR changes suggestion to `filter_map` instead of `flat_map` when mapping to `Option`, because it is more natural Also here are some questions: * If expression has type which implements `Iterator` trait (`match_trait_method(cx, expr, &paths::ITERATOR) == true`), how can I get type of iterator elements? Currently I use return type of closure inside `map`, but probably it is not good way * I would like to change suggestion range to cover only `.map(...).flatten()`, that is from: ``` let _: Vec<_> = vec![5_i8; 6].into_iter().map(|x| 0..x).flatten().collect(); ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try using `flat_map` instead: `vec![5_i8; 6].into_iter().flat_map ``` to ``` let _: Vec<_> = vec![5_i8; 6].into_iter().map(|x| 0..x).flatten().collect(); ^^^^^^^^^^^^^^^^^^^^^^^^ help: try using `flat_map` instead: `.flat_map(|x| 0..x)` ``` Is it ok? * Is `map_flatten` lint intentionally in `pedantic` category, or could it be moved to `complexity`? changelog: Handle mapping to Option in [`map_flatten`](https://rust-lang.github.io/rust-clippy/master/index.html#map_flatten) lint

@ghost

Rollup of 6 pull requests Successful merges: - #5725 (should_impl_trait - ignore methods with lifetime params) - #5837 (needless_collect: catch x: Vec<_> = iter.collect(); x.into_iter() ...) - #5846 (Handle mapping to Option in `map_flatten` lint) - #5848 (Add derive_ord_xor_partial_ord lint) - #5852 (Add lint for duplicate methods of trait bounds) - #5856 (Remove old Symbol reexport) Failed merges: r? @ghost changelog: rollup

@ghost

Rollup of 6 pull requests Successful merges: - #5725 (should_impl_trait - ignore methods with lifetime params) - #5837 (needless_collect: catch x: Vec<_> = iter.collect(); x.into_iter() ...) - #5846 (Handle mapping to Option in `map_flatten` lint) - #5848 (Add derive_ord_xor_partial_ord lint) - #5852 (Add lint for duplicate methods of trait bounds) - #5856 (Remove old Symbol reexport) Failed merges: r? @ghost changelog: rollup

@ghost

Rollup of 5 pull requests Successful merges: - #5837 (needless_collect: catch x: Vec<_> = iter.collect(); x.into_iter() ...) - #5846 (Handle mapping to Option in `map_flatten` lint) - #5848 (Add derive_ord_xor_partial_ord lint) - #5852 (Add lint for duplicate methods of trait bounds) - #5856 (Remove old Symbol reexport) Failed merges: r? @ghost

@ghost

Rollup of 5 pull requests Successful merges: - #5837 (needless_collect: catch x: Vec<_> = iter.collect(); x.into_iter() ...) - #5846 (Handle mapping to Option in `map_flatten` lint) - #5848 (Add derive_ord_xor_partial_ord lint) - #5852 (Add lint for duplicate methods of trait bounds) - #5856 (Remove old Symbol reexport) Failed merges: r? @ghost changelog: rollup

rust-highfive assigned yaahc Jul 25, 2020

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Jul 25, 2020

yaahc requested a review from flip1995 July 25, 2020 17:44

rust-highfive assigned flip1995 and unassigned yaahc Jul 26, 2020

flip1995 reviewed Jul 29, 2020

View reviewed changes

clippy_lints/src/methods/mod.rs Outdated Show resolved Hide resolved

tests/ui/map_flatten.rs Show resolved Hide resolved

clippy_lints/src/methods/mod.rs Outdated Show resolved Hide resolved

flip1995 added S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties labels Jul 29, 2020

flip1995 reviewed Jul 29, 2020

View reviewed changes

tests/ui/map_flatten.rs Show resolved Hide resolved

Handle mapping to Option in map_flatten lint

a427c99

flip1995 reviewed Jul 30, 2020

View reviewed changes

Review fixes

d4ba561

flip1995 mentioned this pull request Aug 4, 2020

Rollup of 6 pull requests #5866

Closed

flip1995 mentioned this pull request Aug 4, 2020

Rollup of 5 pull requests #5868

Merged

bors merged commit 378ba2e into rust-lang:master Aug 4, 2020

ghost mentioned this pull request Dec 2, 2021

Upgrade map_flatten to complexity #8054

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Handle mapping to Option in `map_flatten` lint #5846

Handle mapping to Option in `map_flatten` lint #5846

Uh oh!

dima74 commented Jul 25, 2020

Uh oh!

rust-highfive commented Jul 25, 2020

Uh oh!

flip1995 commented Jul 26, 2020

Uh oh!

flip1995 left a comment •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dima74 commented Jul 30, 2020

Uh oh!

flip1995 left a comment

Uh oh!

flip1995 Jul 30, 2020

Uh oh!

flip1995 Jul 30, 2020

Uh oh!

flip1995 commented Aug 4, 2020

Uh oh!

bors commented Aug 4, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

	span_lint_and_sugg(
	cx,
	UNNECESSARY_FOLD,
	fold_span.with_hi(expr.span.hi()),

Handle mapping to Option in map_flatten lint #5846

Handle mapping to Option in map_flatten lint #5846

Uh oh!

Conversation

dima74 commented Jul 25, 2020

Uh oh!

rust-highfive commented Jul 25, 2020

Uh oh!

flip1995 commented Jul 26, 2020

Uh oh!

flip1995 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dima74 commented Jul 30, 2020

Uh oh!

flip1995 left a comment

Choose a reason for hiding this comment

Uh oh!

flip1995 Jul 30, 2020

Choose a reason for hiding this comment

Uh oh!

flip1995 Jul 30, 2020

Choose a reason for hiding this comment

Uh oh!

flip1995 commented Aug 4, 2020

Uh oh!

bors commented Aug 4, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Handle mapping to Option in `map_flatten` lint #5846

Handle mapping to Option in `map_flatten` lint #5846

flip1995 left a comment •

edited

Loading